Skip to content

structaccess: match encoding/json on ambiguous embedded fields - #6468

Draft
denik wants to merge 11 commits into
denik/structaccess-isflattenedembedfrom
denik/structaccess-embed-ambiguity
Draft

structaccess: match encoding/json on ambiguous embedded fields#6468
denik wants to merge 11 commits into
denik/structaccess-isflattenedembedfrom
denik/structaccess-embed-ambiguity

Conversation

@denik

@denik denik commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Stacked on #6467.

encoding/json drops an embedded field name that two structs declare at the same depth, rather than picking one. Get, Set and ValidatePattern picked the first match, so a caller could read and write a field that is never serialized. All three now report it as not found.

Also: ambiguity is decided from the type, not the value (a nil vs non-nil embedded pointer no longer changes whether a path resolves), a diamond embedding counts as ambiguous, and a cyclic embedding is not walked twice.

This pull request and its description were written by Isaac.

@denik
denik force-pushed the denik/structaccess-embedded-lookup branch from 7cb774c to 79f9b61 Compare September 2, 2026 09:40
@denik
denik force-pushed the denik/structaccess-embed-ambiguity branch from 0bf400f to 0324991 Compare September 2, 2026 09:40
@denik
denik force-pushed the denik/structaccess-embedded-lookup branch from 79f9b61 to 69ae684 Compare September 3, 2026 11:50
@denik
denik force-pushed the denik/structaccess-embed-ambiguity branch 3 times, most recently from 1e961c6 to f09d9ad Compare September 4, 2026 15:59
@denik
denik force-pushed the denik/structaccess-embedded-lookup branch from 69ae684 to 7e6e8da Compare September 4, 2026 16:10
@eng-dev-ecosystem-bot

eng-dev-ecosystem-bot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: 366dcc9

Run: 34133673485

Env 💚​RECOVERED ✅​pass 🙈​skip Time
💚​ aws linux 1 275 15 11:14
💚​ aws windows 1 277 13 7:04
💚​ azure linux 1 274 15 10:13
💚​ azure windows 1 276 13 7:54
💚​ gcp linux 1 275 15 10:32
💚​ gcp windows 1 277 13 7:51
Test Name aws linux aws windows azure linux azure windows gcp linux gcp windows
💚​ TestAccept 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
Top 13 slowest tests (at least 2 minutes):
duration env testname
4:06 azure windows TestAccept
4:03 aws windows TestAccept
3:54 gcp windows TestAccept
3:54 aws linux TestAccept
3:50 azure linux TestAccept
3:48 gcp linux TestAccept
3:13 azure windows TestFilerWorkspaceFilesExtensionsStat
2:57 aws windows TestFilerWorkspaceFilesExtensionsDelete
2:48 azure windows TestFilerWorkspaceFilesExtensionsRead
2:41 azure windows TestFilerWorkspaceFilesExtensionsReadDir
2:26 gcp linux TestFilerWorkspaceFilesExtensionsReadDir
2:11 azure windows TestLock
2:11 gcp windows TestImportDirWithOverwriteFlag

Base automatically changed from denik/structaccess-embedded-lookup to main September 7, 2026 10:16
@denik
denik force-pushed the denik/structaccess-embed-ambiguity branch from f09d9ad to d3bcbec Compare September 7, 2026 13:33
denik added 11 commits September 7, 2026 16:31
When two embedded structs declare one json name at the same depth, encoding/json calls that
ambiguous and omits the field entirely. Get, Set and ValidatePattern picked the first match,
so a caller could read and write a field that is never serialized. All three now report it as
not found, which is what json does with it.

Co-authored-by: Isaac
The repeated json tag is what the fixture exists to exercise.

Co-authored-by: Isaac
From the review: a struct embedding a pointer to itself sent the embedded-field search round
forever whenever the key was not found at all. Both the value and the type walk now skip a
type they have already visited.

Co-authored-by: Isaac
From the review: the cycle guard deduplicated types globally, so a diamond -- two embeds
reaching one type, putting the name at the same depth twice -- was only visited once and
resolved to a field encoding/json omits. Types are now excluded only from earlier levels, so
the two paths within one level produce the two matches that make it ambiguous.

Co-authored-by: Isaac
From the review: two embedded pointers declaring one json name at the same depth make it a name
encoding/json omits, but the value walk skips a nil embed, so whether the name resolved depended
on which pointers happened to be set. Get and Set could reach a field ValidatePattern rejects.

The value walk now asks the type walk first, so all three agree on which names exist.

Co-authored-by: Isaac
… index

Two disagreements with encoding/json, both found by the new agreement tests, and both
fixed by moving field resolution wholly onto the type and following the index chain it
produces -- which is how encoding/json itself resolves a name.

A name declared behind a nil embedded pointer and again deeper down resolved to the deeper
field. encoding/json picks the shallower declaration and then serializes nothing, because
the pointer is nil, so the deeper field is a field the wire format never carries: Get
returned a value that could not be sent and Set wrote where nothing would read. Resolving
on the type and then walking the value means a nil pointer on the winning path reads as an
absent field. Comparing the owner *type* is not enough, because one struct type can be
reachable by two paths.

An anonymous field carrying a json name is a named field to encoding/json -- it serializes
as a nested object under that name -- but the embedded search flattened it anyway. So
"value" resolved on a type that emits {"leaf":{"value":...}}, while "leaf.value", the path
that is actually on the wire, did not resolve at all. Both directions now agree.

The index-chain resolution also replaces the parallel breadth-first walk over values, so
the two searches can no longer drift: findFieldInStruct, embeddedStructs and
embeddedStructTypes are gone.

Co-authored-by: Isaac
Three more from the review, all in name resolution:

At one depth, encoding/json prefers the field whose json tag names it over one that merely
has the matching Go field name; only a genuine tie is ambiguous. The search counted both as
matches and reported the name as not found, so a field the wire format does carry was
unreachable.

A field whose tag sets only an option -- `json:",omitempty"` -- has no json name, so
encoding/json serializes it under its Go field name. The search matched tag names only, so
such a field could not be resolved at all, while structwalk already emitted it under the Go
name: the two disagreed about a field that is plainly on the wire.

Only an anonymous *struct* is promoted. An embedded scalar, slice or interface is a member
named after its type, but IsFlattenedEmbed called it an embed, so structwalk and structdiff
placed its contents at the parent path.

No resource type has any of these shapes: the refschema golden is unchanged.

Co-authored-by: Isaac
…/json does

encoding/json walks an embedded type once per level however many members reach it, so a name
declared *below* a type reached by two routes is not ambiguous -- it resolves along the first
route. A name the duplicated type declares itself is ambiguous, and json serializes neither.

The search treated every route as independent, so it called the first case ambiguous and
reported a name as not found that the wire format does carry. Verified against json rather
than reasoned about: a diamond over the declaring type marshals to {}, while a diamond one
level above it marshals to {"value":"left"}.

The internal/readonly skip keeps its existing behaviour, with a comment recording that it
diverges from encoding/json: such a field shadows a same-named field further down, so
skipping it lets the deeper one win. resources.App is the live example. Rejecting the name
outright instead would make ${resources.apps.*.url} unresolvable, so that is a decision
about what internal means rather than a fix to make here.

Co-authored-by: Isaac
The fifth review round claimed a remaining tagged/untagged bug for a repeated embedded type.
It does not reproduce: the counterexample used omitempty fields left at their zero value, so
"omitted because empty" was indistinguishable from "not serialized at all". With values
populated, all four combinations agree.

They are worth keeping, since the reasoning is easy to get wrong in either direction, so the
table asserts each against encoding/json rather than against a hand-written expectation: a
repeated type declaring the name itself is annihilated, its tagged name losing to a sibling's
untagged X under "X", a repeated untagged name not colliding with the tagged one at all, and
the shallower of two routes winning.

Co-authored-by: Isaac
encoding/json skips a field only when its json tag is exactly "-". A tag whose name part is
"-" followed by options -- json:"-,omitempty" -- names the field "-" and serializes it like
any other name. structtag's parsed name reports "-" for both, so every caller that branched
on the parsed name conflated them: structaccess could not resolve such a field, and
structwalk and structdiff left it out.

structaccess.IsSkippedField now makes the distinction from the raw tag, and the four packages
share it. The structwalk fixture already had two such fields, added as "fixture for odd tag
handling"; its expectation asserted they were skipped, which is what encoding/json does with
json:"-" and not with what they actually carry. Verified against json.Marshal:
{"-":"o","kept":"k"}.

No resource type has the shape -- the refschema golden is unchanged.

Co-authored-by: Isaac
Without it the test only confirms Set wrote to some field json serializes.
If Get resolved to a different (deeper) field the round-trip would be wrong
and all existing assertions would still pass.
@denik
denik changed the base branch from main to denik/structaccess-isflattenedembed September 7, 2026 14:33
@denik
denik force-pushed the denik/structaccess-embed-ambiguity branch from d3bcbec to 366dcc9 Compare September 7, 2026 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants